Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: allow M1 building by removing asm sha on aarch64 #1444

Closed
wants to merge 8 commits into from
Closed

feat: allow M1 building by removing asm sha on aarch64 #1444

wants to merge 8 commits into from

Conversation

cryptonemo
Copy link
Collaborator

For people that need this now -- here are pretty minimal changes to get proofs building on the Apple M1 platform. This isn't ideal because it removes asm usage on aarch64, but works for now. Some tests fail, so patching those up is next.

Build as follows:

LIBRARY_PATH=/opt/homebrew/lib cargo +nightly build --release --all

@cryptonemo
Copy link
Collaborator Author

Builds have not yet been tested through api and ffi.

@cryptonemo
Copy link
Collaborator Author

That reminds me, it's about time to setup the aarch CI for the builds, to perhaps see the failures.

Related to #1353

This was referenced Apr 7, 2021
Closed
Closed
@cryptonemo
Copy link
Collaborator Author

Resolves #1442

@dignifiedquire
Copy link
Contributor

dignifiedquire commented Apr 8, 2021 •
edited
Loading

ideally instead of disabling it, we should port the working code from the official sha2 impl for aarch64 over

@cryptonemo
Copy link
Collaborator Author

All tests are passing on the M1 locally ... albeit ... very ... slow ... ly ...

@cryptonemo cryptonemo force-pushed the m1-portable branch 4 times, most recently from 65ba2be to 87020e4 Compare April 14, 2021 20:31
@cryptonemo
Copy link
Collaborator Author

Good news: CI has been enabled for aarch64.

Bad news: Testing --all with blst takes ~1 hour 15 minutes, and with pairing takes ~1 hour 45 minutes (both using the arm.large instances).

While I'd like to know that tests pass on aarch64 on a regular basis, we may have to consider constraining the tests run to a much smaller subset (and then perhaps drop back to the arm.medium instances). If anyone has thoughts, feel free to chime in. Worst case, we could start with what we have now and scale back over time as needed.

@cryptonemo
Copy link
Collaborator Author

Hah, nevermind my previous comment. So both configurations pass in ~20 minutes after enabling --release. The last thing I'd like to try is find if the arm.medium instances are good enough for that.

@cryptonemo
Copy link
Collaborator Author

~20 minutes on arm.large and ~40 minute test runs on arm.medium shows a linear speed-up with increasing core counts. For now, ~40 minutes on the presumably cheaper arm.medium instances is acceptable.

@cryptonemo
Copy link
Collaborator Author

Resolves #1353

@dignifiedquire dignifiedquire mentioned this pull request Apr 15, 2021
Closed
@ribasushi
Copy link

This isn't ideal because it removes asm usage on aarch64

I run some production infra on ARM in AWS, would be great if you can give me a heads up if this ends up shipping in a stable release, so I can migrate to x86.

@cryptonemo cryptonemo force-pushed the m1-portable branch 2 times, most recently from ad5219b to 418406e Compare April 21, 2021 11:48
dignifiedquire
dignifiedquire reviewed Apr 26, 2021
View reviewed changes
sha2raw/Cargo.toml Outdated
[features]
[target."cfg(target_arch = \"aarch64\")".features]
default = []
[target."cfg(not(target_arch = \"aarch64\"))".features]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still needs restriction to target_os = "macos"

@cryptonemo
Copy link
Collaborator Author

NOTE: This PR is on hold because it disables asm for aarch64 across the board and not for just Apple M1 machines where it isn't supported yet. The issue is that the asm feature of sha2 is used throughout our code as well as dependencies and properly disabling it for this particular arch/os appears troublesome. Unfortunately, until we can find a reasonable solution for this, this can't be merged.

@cryptonemo cryptonemo mentioned this pull request Apr 29, 2021
Closed
@cryptonemo
Copy link
Collaborator Author

Related work: RustCrypto/hashes#261

@cryptonemo
Copy link
Collaborator Author

Darwin CI failure related to #1454 and #1450 (and unrelated to this work)

@cryptonemo
Copy link
Collaborator Author

Closing in favor of #1455 and #1457

@cryptonemo cryptonemo closed this May 6, 2021
@cryptonemo cryptonemo deleted the m1-portable branch May 6, 2021 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dignifiedquire dignifiedquire dignifiedquire left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cryptonemo @dignifiedquire @ribasushi